Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

READY FOR REVIEW: Adapt output declarations #7244

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

simoxb
Copy link

@simoxb simoxb commented Dec 18, 2024

When adapting nf-core code, I realized that some modules declare their output paths using single-quoted strings (e.g. path('*.txt')), while others use double-quoted strings (like path("*.txt")). When adapting the code, this leads to inconsistencies:

Within single quoted outputs, process variables (such as meta.id) cannot be referenced using the usual mechanism ${meta.id}. This makes the code significantly harder to adapt, since using this reference mechanism in single-quoted strings leads to a (not very helpful) "Output file not found" error. I also have not found any helpful information on how to debug this problem, so it took quite some time to fix. Changing output declarations to double-quoted strings everywhere would not bring any disadvantages I'm aware of, but just enable better adaptability because the usual reference mechanism for nextflow process variables with `$' could be used in all output declarations.

Especially the inconsistent use of both single- and double-quoted strings in the different modules makes no sense. I would therefore propose to change all output path declarations to double-quoted strings.

PR checklist

  • [ x] This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the module conventions in the contribution docs
  • If necessary, include test data in your PR.
  • [ x] Remove all TODO statements.
  • Emit the versions.yml file.
  • [ x] Follow the naming conventions.
  • Follow the parameters requirements.
  • [ x] Follow the input/output options guidelines.
  • Add a resource label
  • Use BioConda and BioContainers if possible to fulfil software requirements.
  • Ensure that the test works with either Docker / Singularity. Conda CI tests can be quite flaky:
    • For modules:
      • nf-core modules test <MODULE> --profile docker
      • nf-core modules test <MODULE> --profile singularity
      • nf-core modules test <MODULE> --profile conda
    • For subworkflows:
      • nf-core subworkflows test <SUBWORKFLOW> --profile docker
      • nf-core subworkflows test <SUBWORKFLOW> --profile singularity
      • nf-core subworkflows test <SUBWORKFLOW> --profile conda

- replaced all output paths declared in single quote ' with double quotes "
@simoxb simoxb changed the title Adapt output declarations READY FOR REVIEW: Adapt output declarations Dec 18, 2024
@bentsherman
Copy link

Requiring double-quoted strings everywhere seems like overkill to me. The Nextflow syntax highlighting makes it clear whether the ${meta.id} is being interpolated

@simoxb
Copy link
Author

simoxb commented Dec 18, 2024

Requiring double-quoted strings everywhere seems like overkill to me. The Nextflow syntax highlighting makes it clear whether the ${meta.id} is being interpolated

Fair enough, over-regulation is also a bad thing. At the same time, in nf-core a lot of things are standardized, which mostly aims to improve the user experience and simplify using or adapting the code. Just think it would not hurt too much to set this as standard in nf-core modules, since not everybody is writing Nextflow code using the syntax highlighting, and also is not aware of the different behaviour of different quotes in output path declarations.

There has also been a bit of a discussion on the nf-core slack about this, btw.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants